Skip to content

Conversation

@mzwang34
Copy link

No description provided.


namespace SeeSharp.Blazor;

public static class DocumentationHelper
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Helper" is a bit too vague, a more descriptive name like DocumentationReader or XmlDocumentationReader would be better

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A short documentation comment for this class that quickly summarizes what it does would also be nice.


public static class DocumentationHelper
{
private static Dictionary<string, string> _loadedXmlDocumentation = new();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static Dictionary<string, string> _loadedXmlDocumentation = new();
private static Dictionary<string, string> loadedXmlDocumentation = new();

Convention in the code base is to not use _ prefix for private members

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is useful beyond the Blazor stuff, so maybe this file should be moved to SeeSharp/Common

}
</div>

@code {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For components with a lot of code, it is generally a good idea to separate that into a code-behind file. See https://learn.microsoft.com/en-us/aspnet/core/blazor/components/?view=aspnetcore-10.0#partial-class-support

(Makes it easier to navigate and also prevents some language server quirks and bugs that unfortunately happen a lot on .razor files :( )


protected override void OnInitialized()
{
integratorTypes = new Type[] {typeof(PathTracer), typeof(VertexConnectionAndMerging)};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of hard-coding these two, you should use reflections to find all non-abstract classes that inherit from Integrator

There's already a utility class for that:

public class TypeFactory<T> where T : class {

So TypeFactory<Integrator>.All should do the trick here ;)

Type type = (member is PropertyInfo p) ? p.PropertyType : ((FieldInfo)member).FieldType;
Type underlyingType = Nullable.GetUnderlyingType(type) ?? type;

if (underlyingType == typeof(int) || underlyingType == typeof(uint))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified a bit by only considering three cases:
Integer types, floating point types, and booleans
For ints and floats, you can then use the highest precision (i.e., long and double) always for the Setting component, but convert the final result to the desired type.
(Optionally, you could also check for overflows and raise an error if the value is out-of-bounds for the lower-precision type)

}
catch (Exception ex)
{
Console.WriteLine($"[DocHelper] Error loading XML: {ex.Message}");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use the Logger class instead of Console.WriteLine. That'll make sure to color the message according to type, and show them in the UI as well

return true;
}

void CopyProperties(object target, object source) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name is misleading because it also copies the fields

}
}

bool LoadIntegratorFromJson(string path) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this and the other similar (de)serialization logic should probably go into a separate helper class to make it reusable

partialPath = Path.ChangeExtension(currentPath, "-partial.exr");
}

while (currentSpp < targetTotalSpp)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FrameBuffer supports continuous writing of intermediate results in exponential steps already:

/// <summary>
/// If set, <see cref="WriteIntermediate" /> and <see cref="WriteContinously" /> only output an image
/// after each power-of-two iteration, and <see cref="SendToTev" /> is only triggered at that rate, too.
/// </summary>
WriteExponentially = 32,

This should be used here

@mzwang34
Copy link
Author

This update separates the logic from the UI and improves the information table display with parameter validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants